[Agent Review Lab - Do not merge] - /agent-review-lab verify testing - #1947
Closed
zweatshirt wants to merge 1 commit into
Closed
[Agent Review Lab - Do not merge] - /agent-review-lab verify testing#1947zweatshirt wants to merge 1 commit into
/agent-review-lab verify testing#1947zweatshirt wants to merge 1 commit into
Conversation
zweatshirt
changed the title
/agent-review-lab verify testing: Component with bug /agent-review-lab verify testing: Component with bug
zweatshirt
changed the title
/agent-review-lab verify testing: Component with bug /agent-review-lab verify testing
Contributor
Bundle sizes [mpdx-react]Compared against 0a0f7c3 No significant changes found |
zweatshirt
commented
Jul 28, 2026
zweatshirt
left a comment
zweatshirt
left a comment
Contributor
Author
There was a problem hiding this comment.
🧪 [LAB] Posted by
/quality:agent-review-lab(sandbox) — not the production review, and does NOT trigger auto-approve.
🔴 Verdict: BLOCKERS FOUND
1 critical blocker must be resolved before merge — the double-submission guard, the component's entire reason to exist, is non-functional. This was proven by an executable scratch test during evidence verification (/agent-review-lab verify), not just inferred from reading the code.
- 🔴 SubmitButton.tsx —
setIsSubmitting(true)is never called, sodisabled={isSubmitting}is alwaysfalse. A scratch test that rendered the button with a pendingonSubmitand assertedtoBeDisabled()failed — the button never disables, so a double-click fires two concurrent submits.
Risk Assessment
MEDIUM — Blast Radius 1 · Complexity 2 · Sensitivity 1 · Recoverability 1 (effort 3, danger 2). Shared-tier component with async correctness logic; 0 importers today.
Dependency Impact
SubmitButton.tsx— 0 dependents (new, unwired); no breaking changes.- Naming collision:
SubmitButtonis already exported byShared/Modal/ActionButtons/ActionButtons.tsxand imported by ~55 files app-wide. This thirdSubmitButtonunderShared/invites confusion.
Evidence Verification (Stage 4C)
- Test command:
yarn jest <file>(auto-detected) - Baseline: OK — the existing suite passes (green) despite the broken guard, which is itself corroboration for the false-confidence finding.
- Candidates verified: 1 of 1 — ✅ REPRODUCED
SubmitButton.tsx:24(guard never disables the button).
Suggested regression test to add (fails today, passes after the fix):
it('disables while in flight and fires onSubmit only once on a double click', async () => {
let resolve;
const onSubmit = jest.fn().mockReturnValue(new Promise((r) => (resolve = r)));
const { getByRole } = render(<SubmitButton onSubmit={onSubmit}>Submit</SubmitButton>);
const button = getByRole('button', { name: 'Submit' });
userEvent.click(button);
await waitFor(() => expect(button).toBeDisabled());
userEvent.click(button);
expect(onSubmit).toHaveBeenCalledTimes(1);
resolve();
});Review Summary
| Agent | Critical | High/Important | Suggestions | Confidence |
|---|---|---|---|---|
| Architecture | 1 | 1 | 2 | High |
| Testing | 1 | 2 | — | High |
| Standards | 1 | 1 | — | High |
| UX | 1 | 1 | 3 | High |
Dismissed Findings (Acknowledged by Developer)
These were reviewed and dismissed by the author before posting; they do not count toward the verdict.
- SubmitButton.tsx:31 — No loading indicator (6.0). Dismissed by: @zweatshirt — "Will add the loading indicator separately"
- SubmitButton.tsx:31 — No
aria-busyduring submission (4.0). Dismissed by: @zweatshirt — "Minor UX, not necessary" - SubmitButton.tsx:31 — Hardcoded
variant="contained", no ButtonProps passthrough (3.0). Dismissed by: @zweatshirt — "Will be implemented later"
Standard mode + verify · 4 agents (Architecture, Testing, Standards, UX) + dependency + evidence verification. This is a sandbox review and does not affect auto-approval.
| }) => { | ||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| const handleClick = async () => { |
Contributor
Author
There was a problem hiding this comment.
[Critical] **Double-submit guard is dead — `setIsSubmitting(true)` is never called.** ✅ REPRODUCED by a scratch test during `verify`.
isSubmitting starts false and the only state write is setIsSubmitting(false) on line 27, so disabled={isSubmitting} (line 31) is always false. The button never disables while onSubmit is in flight — a user can double-click and fire two concurrent submits, the exact "two donations from one intent" bug the doc comment promises to prevent.
Evidence: rendering with a pending onSubmit and asserting expect(button).toBeDisabled() fails — the rendered <button> has no disabled attribute mid-flight. The existing test is green, so nothing catches this.
Fix (also resolves the missing try/finally below):
const handleClick = async () => {
setIsSubmitting(true);
try {
await onSubmit();
} finally {
setIsSubmitting(false);
}
};Flagged by all 4 agents (Architecture 9, Testing 9, UX 9, Standards 8).
|
|
||
| const handleClick = async () => { | ||
| // Runs the submit handler and re-enables the button when it settles. | ||
| await onSubmit(); |
Contributor
Author
There was a problem hiding this comment.
[Important] **No `try/finally` — the error path bricks the button.** `await onSubmit()` isn't wrapped. Today a rejection is an unhandled promise rejection; once the guard above is fixed *without* a `finally`, a failed submit (network/GraphQL error) leaves `isSubmitting === true` forever — button stuck disabled, no retry. The `finally` in the Critical fix above resolves this. Per repo UX rules, do **not** add a manual snackbar — the global Apollo error link already toasts GraphQL/network errors.
| import { SubmitButton } from './SubmitButton'; | ||
|
|
||
| describe('SubmitButton', () => { | ||
| it('calls onSubmit when clicked', async () => { |
Contributor
Author
There was a problem hiding this comment.
[Important] **Test gives false confidence — none of the documented contract is covered.** It asserts only `onSubmit` was called once after one click; it never issues a second click and never inspects `disabled`, so it passes whether or not the guard works. Baseline confirmed this suite is green against the broken implementation. Add a double-click-while-pending regression test (see the suggested test in the review summary) so the guard can't silently break again.
| * is in flight the button disables itself, so a second click cannot fire a | ||
| * duplicate request (e.g. creating two donations from one intent). | ||
| */ | ||
| export const SubmitButton: React.FC<SubmitButtonProps> = ({ |
Contributor
Author
There was a problem hiding this comment.
[Medium] **Naming collision / duplication.** `SubmitButton` is already exported by `Shared/Modal/ActionButtons/ActionButtons.tsx` and imported by ~55 files app-wide (plus a local one in `HrTools/SalaryCalculator/StepNavigation`). A third `SubmitButton` sitting directly under `Shared/` is the most discoverable location and will be confused with the adopted one on auto-import. Rename (e.g. `AsyncSubmitButton`) or fold the async-guard behavior into the existing component — cheap now with 0 importers, expensive later.
| <SubmitButton onSubmit={onSubmit}>Submit</SubmitButton>, | ||
| ); | ||
|
|
||
| await userEvent.click(getByRole('button', { name: 'Submit' })); |
Contributor
Author
There was a problem hiding this comment.
[Medium] **`await userEvent.click(...)` violates the repo checklist** ("Never `await` `userEvent` calls"). Drop the `await`. Beyond style, awaiting the click flushes the microtask queue before any assertion, which is part of why the in-flight disabled state couldn't be observed — making the guard test easy to omit.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Suggestion] Named `SubmitButton` and semantically submits, but renders a default `type="button"` wired to `onClick` — it won't respond to Enter inside a form and bypasses Formik's `handleSubmit`. Either commit to `type="submit"` semantics (and align with Formik's `isSubmitting`) or rename to reflect it's a generic async action button.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Dismissed by author: Will add the loading indicator separately] [Medium] No loading indicator during submission. Even once the disabled state works, a greyed-out button is a subtle cue with no spinner; on a slow request users may perceive it as unresponsive. MUI v7 `` gives spinner + disable + `aria-busy` in one.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Dismissed by author: Minor UX, not necessary] [Suggestion] No `aria-busy` / live-region signal during submission, so screen-reader users get no announcement that a submit is in progress. Resolved for free by the MUI `loading` prop.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Dismissed by author: Will be implemented later] [Suggestion] `variant="contained"` is hardcoded and no `ButtonProps` (color, `type`, `sx`, `data-testid`) are forwarded, limiting reuse for a `Shared/` component. Consider spreading `...rest` of `ButtonProps` with `variant` defaulting to `contained`.
zweatshirt
commented
Jul 29, 2026
zweatshirt
left a comment
zweatshirt
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — Verdict: 🔴 BLOCKERS FOUND
1 blocker must be resolved before merge. Standard mode: 4 review agents (Architecture, Testing, Standards, UX) + dependency analysis, with a consolidated consensus pass.
🔴 Blocker
- Double-submit guard is inert —
setIsSubmitting(true)is never called, so the button never disables and the component completely fails its stated double-submission guard (SubmitButton.tsx:24). Unanimous across all 4 agents. See inline comment.
Risk Assessment
LOW — Blast Radius 1 · Complexity 1 · Sensitivity 1 · Recoverability 0 → effort 2, danger 1. A LOW risk gate means the change is reversible and low-sensitivity — it does not clear the correctness blocker; per-finding severity is independent of the gate.
Dependency Impact
- 0 dependents — the new component is currently unused (dead code).
- Name collision (informational):
SubmitButtonalready exists atShared/Modal/ActionButtons/ActionButtons.tsx:19(~60–75 importers) andHrTools/SalaryCalculator/StepNavigation/StepNavigation.tsx:92. No compile break (distinct paths); consolidation/confusion risk only.
Findings posted inline (active)
| # | Sev | File:Line | Finding |
|---|---|---|---|
| 1 | 9.0 🔴 | SubmitButton.tsx:24 | Guard inert — setIsSubmitting(true) never called |
| 2 | 7.0 | SubmitButton.test.tsx:7 | Test can't detect the broken guard (false confidence) |
| 3 | 6.5 | SubmitButton.tsx:26 | No try/finally — rejection strands button + unhandled rejection |
| 4 | 6.5 | SubmitButton.tsx:18 | Duplicate/one-off Shared component; name collision |
| 6 | 5.0 | SubmitButton.test.tsx:13 | Test awaits userEvent.click (repo rule) |
Dismissed Findings (Acknowledged by Developer)
Reviewed and dismissed by the author before posting; they do not count toward the verdict.
- [5.5] SubmitButton.tsx:31 — No loading spinner / in-flight affordance. Dismissed by: @zweatshirt — "Loading spinner will exist in its own PR"
- [4.5] SubmitButton.tsx:4 — Narrow prop API / hardcoded variant. Dismissed by: @zweatshirt — "Intentional"
- [4.0] SubmitButton.tsx:31 — Missing
aria-busyfor in-flight state. Dismissed by: @zweatshirt — "Intentional" - [3.5] SubmitButton.tsx:31 — Named "SubmitButton" but no
type="submit". Dismissed by: @zweatshirt — "Intentional"
| }) => { | ||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| const handleClick = async () => { |
Contributor
Author
There was a problem hiding this comment.
[Critical] **Double-submit guard is inert.** `setIsSubmitting(true)` is never called, so `isSubmitting` stays `false` and `disabled={isSubmitting}` (line 31) is always `false` — the button never disables mid-flight and the documented guard does nothing (a rapid double-click fires `onSubmit` twice, e.g. two donations from one intent). Set the flag before awaiting and reset in `finally`:
const handleClick = async () => {
setIsSubmitting(true);
try {
await onSubmit();
} finally {
setIsSubmitting(false);
}
};Consensus: all 4 review agents (Architecture, UX 9.0; Testing, Standards 8.5). This same fix also resolves the error-handling finding below.
| import { SubmitButton } from './SubmitButton'; | ||
|
|
||
| describe('SubmitButton', () => { | ||
| it('calls onSubmit when clicked', async () => { |
Contributor
Author
There was a problem hiding this comment.
[Important] **Test cannot detect a broken guard — false confidence.** The only test clicks once and asserts `onSubmit` was called once; it never exercises the disable-while-in-flight guard (the component's purpose) or the rejection path, so it passes against the broken implementation — which is why the blocker shipped. Add deferred-promise tests: (a) button becomes `disabled` while `onSubmit` is pending; (b) a second click during flight does **not** fire a second `onSubmit`; (c) button re-enables after both resolve and reject. _Important findings (7.0–7.9) can't be dismissed via `/dismiss`; they don't block merge on their own but are strongly recommended._
|
|
||
| const handleClick = async () => { | ||
| // Runs the submit handler and re-enables the button when it settles. | ||
| await onSubmit(); |
Contributor
Author
There was a problem hiding this comment.
[Medium] **No error handling around `await onSubmit()`.** There's no `try/finally`, so a rejected `onSubmit` skips `setIsSubmitting(false)` and surfaces as an unhandled promise rejection; once the guard bug above is fixed, a failed submit leaves the button **permanently disabled**. The `try/finally` in the blocker fix resolves this. Don't add a manual snackbar — the global Apollo error link already toasts GraphQL/network failures.
| * is in flight the button disables itself, so a second click cannot fire a | ||
| * duplicate request (e.g. creating two donations from one intent). | ||
| */ | ||
| export const SubmitButton: React.FC<SubmitButtonProps> = ({ |
Contributor
Author
There was a problem hiding this comment.
[Medium] **Duplicate/one-off Shared component.** A `SubmitButton` export already exists at `src/components/Shared/Modal/ActionButtons/ActionButtons.tsx:19` (~60–75 importers), and the repo already standardizes on `disabled={isSubmitting}` from Formik plus MUI-lab `LoadingButton` for spinner+disable. Adding a third, identically-named mechanism to `Shared/` conflicts with the rule "don't add one-off components to `Shared/`." Worth a human design decision on whether this component should exist / be named this.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Dismissed by author: Loading spinner will exist in its own PR] [Medium] No loading affordance. Even once disabling works, there's no spinner. The repo pattern pairs a disabled submit with `startIcon={submitting ? : undefined}` (e.g. `SubmitModal.tsx:123`, `GoalsList.tsx:68`) or `LoadingButton`.
| <SubmitButton onSubmit={onSubmit}>Submit</SubmitButton>, | ||
| ); | ||
|
|
||
| await userEvent.click(getByRole('button', { name: 'Submit' })); |
Contributor
Author
There was a problem hiding this comment.
[Medium] Test **awaits `userEvent.click`**, violating the documented repo rule "Never `await` `userEvent` calls" (~97% of the codebase does not). Drop the `await` and move the wait into `waitFor`/`findBy` on the assertion.
| import React, { useState } from 'react'; | ||
| import { Button } from '@mui/material'; | ||
|
|
||
| interface SubmitButtonProps { |
Contributor
Author
There was a problem hiding this comment.
[Dismissed by author: Intentional] [Suggestion] Narrow prop API / hardcoded `variant`. The component accepts only `onSubmit` and `children` and hardcodes `variant="contained"`. Extending `ButtonProps` and forwarding `...props` (plus OR-ing an external `disabled`) would make the Shared button themeable/sizable.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Dismissed by author: Intentional] [Suggestion] Add `aria-busy={isSubmitting}` once the in-flight state works, so screen readers announce "submitting" rather than relying on a spinner alone.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Dismissed by author: Intentional] [Suggestion] Named "SubmitButton" but never sets `type="submit"` (MUI `Button` defaults to `type="button"`). Inside a `` it won't submit on Enter; either expose `type` via prop passthrough or document it as an onClick action button.
zweatshirt
commented
Jul 29, 2026
zweatshirt
left a comment
zweatshirt
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — ⛔ BLOCKERS FOUND (1)
Risk: 2/10 LOW · Mode: standard · Agents: Architecture, Testing, Standards, UX + dependency analysis + adversarial cross-examination
The one thing that must change
SubmitButton never disables itself: setIsSubmitting(true) is never called, so disabled={isSubmitting} is a constant false and the double-submission guard — the component's entire stated purpose — does nothing. A double-click fires onSubmit twice (the exact "two donations from one intent" the JSDoc claims to prevent). All four agents flagged it independently (consensus severity 8.5). It also fails the Standards checklist item "Submit buttons are disabled while isSubmitting is true." See the inline comment on handleClick.
Fix (also handles the rejection path — F2):
const handleClick = async () => {
setIsSubmitting(true);
try {
await onSubmit();
} finally {
setIsSubmitting(false);
}
};Also worth addressing (non-blocking)
- Medium — The lone test never exercises the guard, so the bug ships green. Add a pending-promise + double-click test that asserts
onSubmitis called once and the button isdisabled. - Medium — Name collision: an existing, unrelated
SubmitButtonalready lives atsrc/components/Shared/Modal/ActionButtons/ActionButtons.tsx(imported by 63 modules); this new one has 0 importers and is currently dead code. Rename, remove, or consolidate. - Suggestions — drop the
awaitonuserEvent.click; widen the prop API for aShared/primitive; delete the misleading comment on line 25.
Rejected in cross-examination
typeprop form-submit footgunButtondefaultstypeto"button", not the native"submit".
Dismiss a non-blocking (severity < 7) finding by replying /dismiss: <reason> on its comment. The blocker (severity ≥ 7) cannot be dismissed.
| }) => { | ||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| const handleClick = async () => { |
Contributor
Author
There was a problem hiding this comment.
[High] **Double-submission guard is non-functional.** `setIsSubmitting(true)` is never called anywhere, so `isSubmitting` is permanently `false` and `disabled={isSubmitting}` (line 31) is dead — the button never disables during an in-flight submission. A double-click fires `onSubmit` twice, the exact duplicate-request harm this component's JSDoc says it prevents. This also fails the Standards checklist item "Submit buttons are disabled while isSubmitting is true." Flagged by all 4 review agents.
Fix (sets the flag before awaiting; finally also fixes the rejection path so an error can't strand the button disabled):
const handleClick = async () => {
setIsSubmitting(true);
try {
await onSubmit();
} finally {
setIsSubmitting(false);
}
};|
|
||
| const handleClick = async () => { | ||
| // Runs the submit handler and re-enables the button when it settles. | ||
| await onSubmit(); |
Contributor
Author
There was a problem hiding this comment.
[Medium] No `try/catch/finally` around `await onSubmit()`. Today a rejected `onSubmit` becomes an unhandled promise rejection out of `handleClick`; once the guard bug above is fixed, a rejection would skip `setIsSubmitting(false)` and leave the button permanently disabled after one transient failure. The `try/finally` in the suggested fix resolves both together.
| * is in flight the button disables itself, so a second click cannot fire a | ||
| * duplicate request (e.g. creating two donations from one intent). | ||
| */ | ||
| export const SubmitButton: React.FC<SubmitButtonProps> = ({ |
Contributor
Author
There was a problem hiding this comment.
[Medium] **Name collision + dead code.** An existing, unrelated `SubmitButton` already lives at `src/components/Shared/Modal/ActionButtons/ActionButtons.tsx` and is imported by 63 modules; this new component has 0 importers. Two `Shared/` components with the same name but different contracts (`onSubmit: () => Promise` here vs. a `type="submit"` `ButtonProps` wrapper there) is a maintainability trap. Decide whether this is meant to replace the existing one, or rename/remove it.
| }; | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} disabled={isSubmitting} variant="contained"> |
Contributor
Author
There was a problem hiding this comment.
[Suggestion] Narrow API for a `Shared/` primitive: `variant="contained"` is hardcoded and there is no `disabled` / `color` / `sx` / `ref` passthrough, so the first reuse will force a fork or prop-creep. (Optional: a `CircularProgress` spinner + `aria-busy` would match the `SubmitModal` pattern — but the canonical `SubmitButton` omits both, so it's a nicety, not a requirement.)
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| const handleClick = async () => { | ||
| // Runs the submit handler and re-enables the button when it settles. |
Contributor
Author
There was a problem hiding this comment.
[Suggestion] This comment restates and misdescribes the code — it says the button "re-enables … when it settles," but the button is never disabled (see the guard bug on `handleClick`), and it only clears the flag on the resolve path, not on rejection. Delete it; the repo prefers sparse comments.
| import { SubmitButton } from './SubmitButton'; | ||
|
|
||
| describe('SubmitButton', () => { | ||
| it('calls onSubmit when clicked', async () => { |
Contributor
Author
There was a problem hiding this comment.
[Medium] This is the only test, and it asserts just one click → one call — it never exercises the component's core guarantee. That is why the double-submission bug ships green. Add a test that holds `onSubmit`'s promise pending, clicks twice, and asserts `onSubmit` was called exactly once and the button is `disabled`; add another asserting it re-enables after the promise settles (and after it rejects). These fail against the current implementation — which is the point.
| <SubmitButton onSubmit={onSubmit}>Submit</SubmitButton>, | ||
| ); | ||
|
|
||
| await userEvent.click(getByRole('button', { name: 'Submit' })); |
Contributor
Author
There was a problem hiding this comment.
[Suggestion] `await userEvent.click(...)` contradicts the documented rule "Never await userEvent calls" (.claude/rules/code-review.md) and repo practice (~97% of `userEvent.click` calls in src/components are not awaited). Drop the `await`.
zweatshirt
commented
Jul 29, 2026
zweatshirt
left a comment
zweatshirt
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Multi-Agent Code Review (re-run) — Verdict: 🔴 BLOCKERS FOUND
Verification re-run on an unchanged diff (commit 95e1caf). 1 blocker must be resolved before merge. dismissed: 4.
✅ Dismissal carry-forward working (Stage 0E)
This run recognized 4 findings the author dismissed in earlier runs and did not re-post them as new comments (previously they reappeared on every run). Recognized via the [Dismissed by author: …] markers on the prior top-level comments — no /dismiss replies were needed.
🔴 Blocker
- Double-submit guard is inert —
setIsSubmitting(true)is never called, so the button never disables (SubmitButton.tsx:24). See inline comment.
Risk Assessment
LOW — Blast Radius 1 · Complexity 1 · Sensitivity 1 · Recoverability 0 → effort 2, danger 1. The LOW gate reflects reversibility/low-sensitivity; it does not clear the correctness blocker.
Dependency Impact
- 0 dependents — the new component is currently unused.
- Name collision (info):
SubmitButtonalready exists atShared/Modal/ActionButtons/ActionButtons.tsx:19(~60–75 importers). No compile break; consolidation risk.
Active findings posted inline
| # | Sev | File:Line | Finding |
|---|---|---|---|
| 1 | 9.0 🔴 | SubmitButton.tsx:24 | Guard inert — setIsSubmitting(true) never called |
| 2 | 7.0 | SubmitButton.test.tsx:7 | Test can't detect the broken guard (false confidence) |
| 3 | 6.5 | SubmitButton.tsx:26 | No try/finally — rejection strands button |
| 4 | 6.5 | SubmitButton.tsx:18 | Duplicate/one-off Shared component; name collision |
| 6 | 5.0 | SubmitButton.test.tsx:13 | Test awaits userEvent.click (repo rule) |
Dismissed Findings (Acknowledged by Developer — carried from prior runs, not re-posted)
Recognized by Stage 0E from earlier [Dismissed by author: …] comments; they do not count toward the verdict and are not posted as new line comments.
- [5.5] SubmitButton.tsx:31 — No loading affordance / spinner. Dismissed by: @zweatshirt — "Loading spinner will exist in its own PR"
- [4.5] SubmitButton.tsx:4 — Narrow prop API / hardcoded variant. Dismissed by: @zweatshirt — "Intentional"
- [4.0] SubmitButton.tsx:31 — Missing
aria-busyfor in-flight state. Dismissed by: @zweatshirt — "Intentional" - [3.5] SubmitButton.tsx:31 — Named "SubmitButton" but no
type="submit". Dismissed by: @zweatshirt — "Intentional"
| }) => { | ||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| const handleClick = async () => { |
Contributor
Author
There was a problem hiding this comment.
[Critical] **Double-submit guard is inert.** `setIsSubmitting(true)` is never called, so `isSubmitting` stays `false` and `disabled={isSubmitting}` (line 31) is always `false` — the button never disables mid-flight and the documented guard does nothing (a rapid double-click fires `onSubmit` twice). Set the flag before awaiting and reset in `finally`:
const handleClick = async () => {
setIsSubmitting(true);
try {
await onSubmit();
} finally {
setIsSubmitting(false);
}
};Consensus: all 4 review agents. This same fix also resolves the error-handling finding below.
| import { SubmitButton } from './SubmitButton'; | ||
|
|
||
| describe('SubmitButton', () => { | ||
| it('calls onSubmit when clicked', async () => { |
Contributor
Author
There was a problem hiding this comment.
[Important] **Test cannot detect a broken guard — false confidence.** The only test clicks once and asserts `onSubmit` was called once; it never exercises the disable-while-in-flight guard or the rejection path, so it passes against the broken implementation. Add deferred-promise tests: (a) button `disabled` while pending; (b) a second click during flight does NOT fire a second `onSubmit`; (c) re-enables after resolve and reject. _Important findings can't be dismissed via `/dismiss`._
|
|
||
| const handleClick = async () => { | ||
| // Runs the submit handler and re-enables the button when it settles. | ||
| await onSubmit(); |
Contributor
Author
There was a problem hiding this comment.
[Medium] **No error handling around `await onSubmit()`.** No `try/finally`, so a rejected `onSubmit` skips `setIsSubmitting(false)` and surfaces as an unhandled rejection; once the guard bug above is fixed, a failed submit leaves the button **permanently disabled**. The `try/finally` in the blocker fix resolves this. Don't add a manual snackbar — the global Apollo error link already toasts failures.
| * is in flight the button disables itself, so a second click cannot fire a | ||
| * duplicate request (e.g. creating two donations from one intent). | ||
| */ | ||
| export const SubmitButton: React.FC<SubmitButtonProps> = ({ |
Contributor
Author
There was a problem hiding this comment.
[Medium] **Duplicate/one-off Shared component.** A `SubmitButton` export already exists at `src/components/Shared/Modal/ActionButtons/ActionButtons.tsx:19` (~60–75 importers), and the repo standardizes on `disabled={isSubmitting}` from Formik plus MUI-lab `LoadingButton`. Adding a third, identically-named mechanism to `Shared/` conflicts with "don't add one-off components to `Shared/`." Worth a human design decision.
| <SubmitButton onSubmit={onSubmit}>Submit</SubmitButton>, | ||
| ); | ||
|
|
||
| await userEvent.click(getByRole('button', { name: 'Submit' })); |
Contributor
Author
There was a problem hiding this comment.
[Medium] Test **awaits `userEvent.click`**, violating the documented repo rule "Never `await` `userEvent` calls" (~97% of the codebase does not). Drop the `await` and move the wait into `waitFor`/`findBy` on the assertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please explain a bullet-point summary of the changes.
List any PRs that this PR is dependent on and any Jira tickets that this PR is related to.
Testing
Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions